Skip to content

Multi rfq send itest #1097

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

GeorgeTsagk
Copy link
Member

@GeorgeTsagk GeorgeTsagk commented Jun 23, 2025

Description

Enhances the multi-rfq itest to cover multi-rfq send functionality. In the following topology

	  /---[sats]--> Erin --[assets]--\
	 /                                \
	/                                  \

 Charlie  -----[sats]-->  Dave  --[assets]---->Fabia

	\                                  /
	 \                                /
	  \---[sats]--> Yara --[assets]--/

we now also get Fabia to pay Charlie back, with amounts that exceed the capacity of each individual channel. We also use a hold invoice to validate that multiple HTLCs were added in different channels.

Related PRs (in order of merge sequence):

@GeorgeTsagk GeorgeTsagk self-assigned this Jun 23, 2025
@GeorgeTsagk GeorgeTsagk force-pushed the multi-rfq-send-itest branch 2 times, most recently from 6165322 to c7dbe80 Compare June 27, 2025 12:10
@ZZiigguurraatt
Copy link

Seems like you should update

https://github.com/GeorgeTsagk/lightning-terminal/blob/c7dbe80f1b6463bb01fe2f94f72cf52b6b6ab9de/itest/litd_custom_channels_test.go#L2973-L2977

to mention the send test, possibly renaming the function name?

@ZZiigguurraatt
Copy link

In the following topology

	  /---[sats]--> Erin --[assets]--\
	 /                                \
	/                                  \

 Charlie  -----[sats]-->  Dave  --[assets]---->Fabia

	\                                  /
	 \                                /
	  \---[sats]--> Yara --[assets]--/

Do we see any value in adding tests where Charlie has more or less channels than Fabia? Like this route seems a bit too simple because both sender and receiver kind of need the same sharding.

I'm also wondering if we should test where Charlie-Dave have asset channels and not sats channels.

I don't think this test should be removed, but I think there should be some more complex network that tests more than what this test can do in another function.

@GeorgeTsagk GeorgeTsagk force-pushed the multi-rfq-send-itest branch 2 times, most recently from 1e79938 to 533a39f Compare July 2, 2025 10:39
@GeorgeTsagk GeorgeTsagk requested review from guggero and ffranr July 2, 2025 10:39
@GeorgeTsagk
Copy link
Member Author

Linter / unit tests fail because of this
https://github.com/lightninglabs/lightning-terminal/actions/runs/16022889778/job/45203816341?pr=1097#step:5:317

accounts/sql_migration_test.go:340:19: undefined: sqldb.MigrationTxOptions (typecheck)

which is not related to this PR, the itests should normally run & pass

@GeorgeTsagk
Copy link
Member Author

The last commit adds a new node to the topology, which always rejects quotes, causing a reliable RFQ negotiation failure this way

The tests pass, but they do now expose some potential bug in the RFQ negotiation code where we hang until timeout if the rfq negotiation is not successful.

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending a couple of nits.

@lightninglabs-deploy
Copy link

@ffranr: review reminder
@GeorgeTsagk, remember to re-request review from reviewers when ready

Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove the replace in go.mod, and some other outstanding comments, otherwise good to go.

@ZZiigguurraatt
Copy link

Need to remove the replace in go.mod, and some other outstanding comments, otherwise good to go.

FYI: I made a branch where I rebased and then updated all the packages. Not sure if you want to use it or re-do it another way.

https://github.com/ZZiigguurraatt/taproot-assets/tree/group-key-addr-part-2%2Bmulti-rfq-send

@ZZiigguurraatt
Copy link

Need to remove the replace in go.mod, and some other outstanding comments, otherwise good to go.

FYI: I made a branch where I rebased and then updated all the packages. Not sure if you want to use it or re-do it another way.

https://github.com/ZZiigguurraatt/taproot-assets/tree/group-key-addr-part-2%2Bmulti-rfq-send

sorry, got this PR mixed up with the other one.

@GeorgeTsagk GeorgeTsagk force-pushed the multi-rfq-send-itest branch 2 times, most recently from b60e3e4 to 7e35182 Compare July 22, 2025 13:35
@GeorgeTsagk GeorgeTsagk requested a review from ffranr July 22, 2025 13:35
@ffranr ffranr added the no-changelog This PR is does not require a release notes entry label Jul 22, 2025
@guggero
Copy link
Member

guggero commented Jul 23, 2025

@GeorgeTsagk can you please rebase this to the tapd-main-branch branch? We can't bump tapd on master branch yet, as we're not ready to release what will become tapd v0.7.0 yet (in case we need to do an emergency minor release of litd).

@ZZiigguurraatt
Copy link

@GeorgeTsagk can you please rebase this to the tapd-main-branch branch? We can't bump tapd on master branch yet, as we're not ready to release what will become tapd v0.7.0 yet (in case we need to do an emergency minor release of litd).

do we want to prio lightninglabs/loop#968 now so that this can be done more cleanly?

@GeorgeTsagk GeorgeTsagk force-pushed the multi-rfq-send-itest branch from 7e35182 to fda2ed7 Compare July 28, 2025 10:41
@GeorgeTsagk GeorgeTsagk changed the base branch from master to tapd-main-branch July 28, 2025 10:41
@GeorgeTsagk
Copy link
Member Author

Rebased on tapd-main-branch

@GeorgeTsagk
Copy link
Member Author

do we want to prio lightninglabs/loop#968 now so that this can be done more cleanly?

We can't merge the loop PR until the tapd PR gets merged, as we have to provide the final go mod ref

@GeorgeTsagk GeorgeTsagk force-pushed the multi-rfq-send-itest branch from fda2ed7 to 9139f5d Compare July 28, 2025 10:43
@guggero guggero force-pushed the tapd-main-branch branch 3 times, most recently from 261d3ec to f3aba31 Compare July 29, 2025 11:13
@guggero
Copy link
Member

guggero commented Jul 30, 2025

Can you rebase this please so we can merge this to fix the tapd CI?

@GeorgeTsagk
Copy link
Member Author

Can you rebase this please so we can merge this to fix the tapd CI?

Hey

Currently blocked on this lightninglabs/loop#968

Then we can finalize this PR and merge

@guggero
Copy link
Member

guggero commented Jul 30, 2025

That loop PR doesn't have to be merged, since we're merging into a temporary side branch here as well (to which the tapd CI currently points).

@GeorgeTsagk GeorgeTsagk force-pushed the multi-rfq-send-itest branch from 9139f5d to b127dd9 Compare July 30, 2025 14:42
@GeorgeTsagk GeorgeTsagk force-pushed the multi-rfq-send-itest branch 2 times, most recently from f1c0012 to 6c6262a Compare July 30, 2025 16:57
@GeorgeTsagk GeorgeTsagk force-pushed the multi-rfq-send-itest branch from 6c6262a to 5cb0205 Compare July 30, 2025 18:31
@GeorgeTsagk
Copy link
Member Author

After addressing some of the latest feedback in the tapd PR I realized that some errors/RPC responses had changed, which required some fixes on this side. It's good now

@GeorgeTsagk GeorgeTsagk merged commit 91bc1ac into lightninglabs:tapd-main-branch Jul 30, 2025
63 of 66 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog This PR is does not require a release notes entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants